strip control characters when writing the cookie header - #900

good catch on the spaces. i've changed it so a maximal run of CR/LF/NUL collapses to a single space, so a folded CRLF now leaves one space rather than a run of them, and updated the expected value to match (sid="abc Injected: evil").

on the second point, agreed, this only neutralises the write-side injection vector and isn't meant to fully validate the name against the token grammar or the value against cookie-octet. i kept the change narrow to the CR/LF/NUL that split the header, matching what BufferedHeader.getValue() already does on read-back. happy to follow up with stricter cookie-name/cookie-value validation as a separate change if you'd like it.

@dxbjavid @arturobernalg Can we take a step back and take look at the problem from a broader view.

But first, I really do not like this

On the classic transport the header is serialised from the raw buffer of the BufferedHeader rather than its getValue()

Does that mean the async transport behaves differently in that regard? If so, this needs to be fixed first and foremost.

Then, is this problem really specific to RFC6265 cookie spec? Should not it be fixed at at the HTTP/1.1 connection layer for headers of any type?

@dxbjavid As far as I can see the behavior of the classic and the async message writers looks consistent

https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/AbstractMessageWriter.java#L86
https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractMessageWriter.java#L87

you're right that the two writers are consistent. both take the FormattedHeader's raw CharArrayBuffer and write it verbatim, so this isn't a classic-vs-async difference, and my wording in the description was misleading on that. the asymmetry is really within the header itself: BufferedHeader.getValue() substitutes CR/LF/NUL with a space, but the buffer the writers serialise keeps them.

agreed too that the proper place for a write-side guard is the HTTP/1.1 header serialisation, so it covers headers of any type rather than just RFC6265 cookies. that lives in httpcore (AbstractMessageWriter), so it'd be a change over there, not here. happy to raise it against httpcomponents-core, either stripping the control characters at serialisation or rejecting a header that carries them, whichever you'd prefer. if you'd rather fix it centrally i can close this one, or leave it as a narrow cookie-spec stopgap until the core change lands.

@dxbjavid @arturobernalg Can we take a step back and take look at the problem from a broader view.

But first, I really do not like this

On the classic transport the header is serialised from the raw buffer of the BufferedHeader rather than its getValue()

Does that mean the async transport behaves differently in that regard? If so, this needs to be fixed first and foremost.

Then, is this problem really specific to RFC6265 cookie spec? Should not it be fixed at at the HTTP/1.1 connection layer for headers of any type?

Agreed. FormattedHeader bypasses the normal header formatting, so this is not cookie-specific. The fix belongs in HttpCore's HTTP/1 serialization layer, covering both classic and async transports.

you're right that the two writers are consistent. both take the FormattedHeader's raw CharArrayBuffer and write it verbatim, so this isn't a classic-vs-async difference, and my wording in the description was misleading on that. the asymmetry is really within the header itself: BufferedHeader.getValue() substitutes CR/LF/NUL with a space, but the buffer the writers serialise keeps them.

agreed too that the proper place for a write-side guard is the HTTP/1.1 header serialisation, so it covers headers of any type rather than just RFC6265 cookies. that lives in httpcore (AbstractMessageWriter), so it'd be a change over there, not here. happy to raise it against httpcomponents-core, either stripping the control characters at serialisation or rejecting a header that carries them, whichever you'd prefer. if you'd rather fix it centrally i can close this one, or leave it as a narrow cookie-spec stopgap until the core change lands.

@dxbjavid I do not mind having an extra safe-guard in RFC6265 cookie policy, but I want to understand if there is a bigger problem at the low transport level. As far as I understand there is already a safe-guard to ensure no non-printable characters can end up in the session buffer. See TextUtils#castAsByte and its usage.

I am actually not sure there is a problem at all.

@dxbjavid It looks like @arturobernalg has found the root cause of the issue. The unchecked conversion from char to byte can only occur when the user has explicitly specified a non-standard charset for HTTP message elements. I personally do not see this as a very severe problem, but if we decide to fix it it should be fixed in core (the session buffer layer).

@ok2c @arturobernalg that matches what i see now. with no charset configured, SessionOutputBufferImpl.writeLine copies the header buffer through ByteArrayBuffer.append, which runs every char through TextUtils.castAsByte, so CR/LF/NUL go out as '?' and the Cookie line can't be split. my test only looked at the FormattedHeader buffer rather than the bytes that actually reach the socket, so the description overstated it for the default setup, sorry about that.

the gap that's left looks to be the encoder path (writeEncoded), which only applies when a non-standard charset has been set explicitly and hands the chars to the CharsetEncoder unchecked. agreed that's low severity and belongs in the session buffer layer in core. i can open a small PR against httpcomponents-core that applies the same substitution there if that's useful. as for this one, i'm happy to keep it as an extra cookie-level safeguard or close it, whichever you prefer.

@ok2c @arturobernalg that matches what i see now. with no charset configured, SessionOutputBufferImpl.writeLine copies the header buffer through ByteArrayBuffer.append, which runs every char through TextUtils.castAsByte, so CR/LF/NUL go out as '?' and the Cookie line can't be split. my test only looked at the FormattedHeader buffer rather than the bytes that actually reach the socket, so the description overstated it for the default setup, sorry about that.

the gap that's left looks to be the encoder path (writeEncoded), which only applies when a non-standard charset has been set explicitly and hands the chars to the CharsetEncoder unchecked. agreed that's low severity and belongs in the session buffer layer in core. i can open a small PR against httpcomponents-core that applies the same substitution there if that's useful. as for this one, i'm happy to keep it as an extra cookie-level safeguard or close it, whichever you prefer.

@dxbjavid
Thanks for confirming. This is already addressed by apache/httpcomponents-core#725, which covers the explicit charset encoder path in both classic and async transports.

@dxbjavid What we can still do in client is to reject cookies with non-printable chars in them as invalid, given sensitive nature of cookies.. Please do feel to re-spin your change-set and please do continue contributing.